Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toggle block comments #4718

Merged
merged 1 commit into from
Feb 27, 2024
Merged

Toggle block comments #4718

merged 1 commit into from
Feb 27, 2024

Conversation

gabydd
Copy link
Member

@gabydd gabydd commented Nov 11, 2022

this is an initial implementation of toggling block comments which fixes #1505 it still needs

  • decision for keybindings, for now I put block comments under space-c-b and line comments to space-c-c
  • if we should have a single command for toggle comment that does either block or line toggle
  • add more block comment tokens to language.toml
  • can we get comment tokens from treesitter
  • right now toggle a block doesn't extend the selection, should it do that? extends for non linewise block comments
  • tests
  • comments
  • add to docs when keybindings are finalized

any other feedback appreciated, this is still a draft and nothing is set in stone

some implementation notes:

  • the command normalizes the commenting across the selections so if one selection is uncommented all selections get commented, this can be changed if needed
  • comments surround the content of each selection ignoring whitespace when commenting and leave any whitespace surrounding block comment tokens when uncommenting
  • i tried to do the same thing as find_first_non_whitespace_character in find_last_non_whitespace_character but apparently Chars doesn't implement DoubleEndedIterator so I just iterate over a reversed range of chars len instead. I don't think this should have performance implications on really long selections, but someone else might know better. pascal figured this out

video of it in action

@gabydd
Copy link
Member Author

gabydd commented Nov 12, 2022

I think I can make it so that there is one command that can toggle block and line comments,, then seperate commands for just block and line toggling, my proposed algorithm is:

if only one of single line token or block tokens exist, toggle that, else do the following:

  • check if all selections are line commented
    • if they are then line toggle
  • check if all selections are block toggled
    • if they are then line toggle
  • check if all selections are single line
    • if they are then line toggle
    • should this check for block comments?
  • else toggle block
    • could potentially also check and skip over already line commented code, but then block toggling would also do line toggling

@gabydd
Copy link
Member Author

gabydd commented Nov 13, 2022

I implemented the above, and then separated line comment toggling into it's own command that is under space+c+l

there is definitely ways to make block commenting smarter, for example comment a block if selection is surrounded by only whitespace and then comment token or uncomment block that is surrounded by other characters then whitespace in a selection(though I think this could actually extend the comment block maybe). This could be saved for future pr if this functionality is wanted.

@kirawi kirawi added the A-helix-term Area: Helix term improvements label Nov 14, 2022
Copy link
Member

@pascalkuthe pascalkuthe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! Comment as discussed in matrix

helix-core/src/comment.rs Outdated Show resolved Hide resolved
This was referenced Nov 27, 2022
@gabydd
Copy link
Member Author

gabydd commented Dec 4, 2022

This PR should now be ready for review, some things to note:

  • selection isn't extended to encompass the comment, wasn't sure how or if I should do this
  • keybindings should be chosen, I know some people wanted to switch to #, I don't have strong opinions about this both are great to me
  • this is pretty much first rust stuff I have done, so if anything seems wrong please don't assume it's not you probably know better then me.
  • this is breaking change to how comment toggling works
    Also want to thank everyone for all the work they have put into helix, using and contributing to it has been a joy
    (the doc failing seems to be based on a failed network call, shouldn't affect this pr)

@gabydd gabydd marked this pull request as ready for review December 4, 2022 06:18
@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. R-breaking-change This PR is a breaking change for some behavior labels Dec 4, 2022
@pascalkuthe
Copy link
Member

pascalkuthe commented Dec 5, 2022

Thanks for working on this! I think block comments are useful to have in particular for languages that do not have line comments.

I had this in my daily driver for a while and have some thoughts. I think automatically switching between line comments and block commenys based upon whether more then 1 char is selectes is nice to have but I found myself frustrated by this as a default.

For languages that support them I almost always want line comments because of the more granular control (comment ten lines, later uncomment only two of them or move some part of the comment elsewhere without having to add new commentnstsrt/end) so I would prefer to keep that as is.
However for languages that do not have line comments by default it should fall back to adding a block comment to every selected lime (same for <space>cl).

I think the automatic option is still useful but perhaphs as a secondary command (maybe mapped to C-C by default and people who prefer it the other way around can swap those two?).

If other people (@archseer whatdo you think?) prefer the default as in this PR I think its fine too as the I keybindings can be swappes in my config. I think the suggesting about using block comments as a line comment fallback should definitely be implement

@pascalkuthe pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Dec 5, 2022
@gabydd
Copy link
Member Author

gabydd commented Dec 5, 2022

Thanks for your input @pascalkuthe it's very helpful, and unless someone else objects I think I will implement it as the default. This seems simpler and more intuitive to the user, I don't have time today to implement this but it was in the back of my mind after I marked it for review so it shouldn't take me too long.

@gabydd
Copy link
Member Author

gabydd commented Dec 22, 2022

sorry for the wait on this I have been pretty busy, the new relevant changes are more inline with @pascalkuthe suggestions(if you have the time to take a look at the new behaviour that would be appreciated, I am sure you are pretty busy right now so it's totally okay if you don't do an actual review)

the key changes are summarized here:

  • toggle comments always does a line comment or uses block as line fallback (this makes it much less of a breaking change)

  • it only does a regular block comment when the selection is already block commented

  • line comment and block comment now both fallback to each other if there token is not available for that language

    • this is a proper fallback in the case of line comments where it comments each line separately

whats left at this point is to add this to the docs and look into making some more meaningful tests

@gibbz00
Copy link
Contributor

gibbz00 commented Dec 25, 2022

How will this affect the default "C-c" => toggle_comments binding?

@gabydd
Copy link
Member Author

gabydd commented Dec 25, 2022

Only effects if there is no line comment-token for a language(falls back to using the block comment tokens to comment each line instead)or if the whole selection is block commented other then that toggle_comments stays the same

@pascalkuthe pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 25, 2022
@pascalkuthe
Copy link
Member

@gabydd Is this ready for review?

@gabydd
Copy link
Member Author

gabydd commented Jun 20, 2023

Yep except it still needs the documentation, the one thing I am still unsure about is if we want to change how the config for block comments is to make it a map to easier add the token for continue comments, or do we think that can be a seperate option?

@pascalkuthe
Copy link
Member

ok thanks for letting me know. I will think about that during review

@gabydd
Copy link
Member Author

gabydd commented Oct 30, 2023

Just fixed the merge conflicts I also squashed the pr to one commit so it's easier to rebase when there are conflicts if we want I can make one commit the code changes and the other the languages.toml changes

@gabydd gabydd force-pushed the toggle-block branch 3 times, most recently from 3b4ea22 to 8146de6 Compare November 19, 2023 19:35
@pascalkuthe
Copy link
Member

(Also closes #4009 but I can't link the issue because the GH UI is terribble)

@gabydd
Copy link
Member Author

gabydd commented Nov 23, 2023

This only fixes that for block comments, I didn't want to change the functionality of line comments in this pr

@gabydd
Copy link
Member Author

gabydd commented Nov 23, 2023

What we could do is for languages without a block comment token, when using the toggle_block_comments it would only add or remove from the start of the selection as if you had a block comment that was the line comment on the left and an empty string on the right

@gabydd
Copy link
Member Author

gabydd commented Dec 5, 2023

redundant_clone seems to have some false positives: rust-lang/rust-clippy#10577

@the-mikedavis
Copy link
Member

the-mikedavis commented Dec 5, 2023

Let's add some #[allow(clippy::redundant_clone)]s arround those for now with a reminder to check if we can remove them when the MSRV is bumped. Looks like the issue isn't solved yet so we might have to keep the allows around for a little while

@daedroza
Copy link
Contributor

Using this since last week and works great, thanks for putting this up!

the-mikedavis
the-mikedavis previously approved these changes Jan 23, 2024
pascalkuthe
pascalkuthe previously approved these changes Jan 23, 2024
@archseer archseer merged commit 26b3dc2 into helix-editor:master Feb 27, 2024
6 checks passed
@gabydd gabydd deleted the toggle-block branch February 27, 2024 18:24
postsolar pushed a commit to postsolar/helix that referenced this pull request Apr 4, 2024
mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request Jun 2, 2024
Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request Jun 7, 2024
smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-command Area: Commands A-core Area: Helix core improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for block comments
8 participants